Feat (ci-rules) : Add AZ-CI-001..004 CI/CD workflow security rules (issue #259 PR 1/3) - #280

Thanks solid structure overall, the N/A-CI-* convention is right, tests are thorough, and the NIST mapping fix (PR.DS-5 → PR.AC-4) looks good. All CI is green. However there is one blocking integration bug plus a few correctness gaps that need fixing before merge:

Blocking

1. The four rules are loaded by the scan engine but can never run.
engine.py glob-loads every az_*.py in scanner/rules/ and calls rule.scan(self.client, self.subscription_id) with an AzureClient (engine.py:71, engine.py:114). The AZ-CI rules declare scan(github_client, owner, repo) three required params so every production scan raises TypeError for all four rules, incrementing RULE_ERRORS_TOTAL and logging 4 errors per scan, while the rules silently never execute. Even if the wiring lands in PR 3/3, this PR must either exclude az_ci_* from the engine loader or provide an engine-compatible adapter, otherwise every scan between the two merges is polluted.

Should fix

2. github_client.py get_workflows() does not paginate.
/repos/{owner}/{repo}/actions/workflows returns 30 items per page by default. Repos with >30 workflows are silently truncated → false negatives on exactly the large repos this feature targets. Follow the Link header or pass per_page=100 + loop.

3. _workflow_common.py collect_uses() misses job-level uses: and crashes on empty jobs.
Reusable workflow calls (jobs.<id>.uses) are never collected, so AZ-CI-003 can't flag an unpinned reusable workflow. Also jobs: {build: null} → None.get("steps") → AttributeError.

4. az_ci_001.py OIDC anywhere in the file suppresses the long-lived-credential finding.
A workflow using azure/login with OIDC and separately referencing ARM_ACCESS_KEY passes clean. The suppression should require the credential to be absent, not merely OIDC to be present. Related: uses_workload_identity() matches the literal string "id-token: write", which breaks on extra spacing or quoted YAML — check the parsed permissions dict instead.

Nits

**5. github_client.py get_workflow_content():** for files >1 MB the contents API returns an empty contentfield; the code decodes""and caches it as valid content, so an oversized workflow scans as compliant instead of UNKNOWN. Treat emptycontentwith a presentdownload_url`/size as a failure.

6. tests/test_rules_ci_workflow.py test_no_token_returns_none_from_get_token: assert token is None or isinstance(token, str) is always true; assert token is None under the cleared env instead.

@emon22-ts good progress: engine exclusion, collect_uses hardening, the 1 MB guard, and the token test are all properly fixed. One blocker remains:

AZ-CI-001 OIDC suppression is still broken, and the new comment claims otherwise. az_ci_001.py says "A workflow using OIDC but also referencing ARM_ACCESS_KEY still fails", but uses_workload_identity() checks only _LONGTERM_CRED_PATTERNS[0] (AZURE_CLIENT_SECRET). OIDC + ARM_ACCESS_KEY/ARM_CLIENT_SECRET/AZURE_STORAGE_KEY/AZURE_PASSWORD still passes clean. Make the suppression require no credential match at all (e.g. gate on not creds in the scan, or check every pattern), and add a regression test for OIDC + ARM_ACCESS_KEY, since the absence of that test is exactly why this slipped.

Smaller items, same push: get_workflows() sends per_page=100 but still doesn't follow the Link header (docstring says "Paginates"; >100 workflows still truncate), uses_workload_identity() ignores job-level permissions so job-scoped id-token: write false-positives, and the four framework JSONs lost their trailing newlines.

Fix the AZ-CI-001 suppression and this is mergeable from my side.

Architecturally this is sound: github_client.py matches the codebase's existing try/except -> logger.error -> None-means-UNKNOWN convention exactly, no RULE_ID collisions against the ~90 existing rules, all 4 playbooks exist and pass bash -n, compliance framework JSON additions match the existing schema, and the YAML-1.1 on: parsing as boolean True is handled correctly (workflow.get("on") or workflow.get(True)) - an easy, common mistake in GitHub Actions tooling that this PR avoids.

But for a compliance scanner, misclassifying a secure config as a violation is a correctness bug, not a nit, and I confirmed 4 real false-positive paths by feeding realistic workflow YAML directly into each rule's scan() rather than just reading the code - all 4 are in code this PR adds. Left inline comments on each with the specific repro.

One more, documented as intentional rather than an oversight (az_ci_003.py's REMEDIATION explicitly calls Docker actions exempt) but worth a decision either way: is_action_pinned unconditionally treats every docker:// reference as pinned, so uses: docker://alpine:latest (a mutable tag) is never flagged - confirmed with a direct repro (0 findings). This is a real gap against the rule's own stated threat model, and OpenSSF Scorecard's Pinned-Dependencies check (cited as this PR's own compliance evidence source) does flag it. Worth confirming whether this is deliberate scope-narrowing for 1/3 or should require @sha256:... digests for Docker actions too.

Also flagging for visibility, not blocking: scanner/engine.py explicitly excludes az_ci_* from the automatic rule loader with a comment claiming a separate CI/CD scan entry point invokes them - I grepped the whole repo (api/, scanner/worker.py, everything) and no such entry point exists anywhere yet, on this branch or dev. Not a bug (nothing crashes), but since this is titled "1/3," worth confirming reviewers know this ships tested logic with no way to actually run it in the product until a follow-up wires it up. Separately, TestGitHubClient only covers trivial success/failure - the Link-header pagination in get_workflows, the GitHub App JWT-auth flow in _get_app_token, and the >1MB download_url branch in get_workflow_content have no coverage, worth closing before 2/3 builds on top of this client.

Full suite (after installing this environment's missing azure-*/prometheus-client/python-dotenv packages, an environment gap unrelated to the PR): 758 passed, 3 skipped. tests/test_rules_ci_workflow.py: 47/47 passed - the existing tests don't catch any of the 4 false positives below because none of them construct a workflow that's secure-by-a-more-specific-mechanism-than-the-naive-check (safe pull_request_target via base-ref checkout, job-level OIDC alongside an unrelated job's real secret, a credential name only in a comment, or per-job explicit least-privilege with no top-level default).

good to go

Emon, the four reported false-positive fixes survived the rebase: PR-head detection is scoped to checkout ref, job-level OIDC is recognized, comment-only credential names are ignored, and fully job-scoped permissions no longer fail merely because the top-level block is absent. CI is green.

I found one remaining permission-model false negative in AZ-CI-002. is_permissions_broad() treats an absent top-level block as safe whenever every job merely has a non-null permissions field; it never evaluates what those job-level values grant. Therefore a workflow where every job declares permissions: write-all, or three sensitive *: write scopes, returns compliant. Likewise, a harmless top-level contents: read plus a job-level permissions: write-all is never examined because only the top-level block is passed into the check. Those are exactly the over-broad token permissions this rule claims to detect.

Please evaluate the effective permissions at both levels: flag write-all/write and the broad-scope threshold in any job override, while preserving the valid per-job least-privilege case. Add regression tests for (1) no top-level block with every job declaring write-all, (2) safe top-level permissions overridden by one broad job, and (3) multiple jobs with narrow explicit permissions remaining clean. Then rerun the suite and request rereview.